Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Automatic access verification for AOAI services to develop and run on CAPI/managed AI resources #2764

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Automatic access verification for AOAI services to develop and run on CAPI/managed AI resources #2764

wants to merge 32 commits into from
+271 −6

Conversation

christian-andersen-msft
Copy link
Contributor

@christian-andersen-msft christian-andersen-msft commented Jan 14, 2025 •
edited by github-actions bot
Loading

Fixes AB#535826

Christian Andersen and others added 17 commits November 26, 2024 14:00
Slice 535826: [AI][Public Preview]Copilot Toolkit: Automatic access v…
1702f14 
…erification for AOAI services to develop and run on CAPI/managed AI resources
Restored SetManagedResourceAuthorization with old parameters
09f9d04
Removed unnessecary variables when verifying Microsoft managed resour…
4119ef8 
…ces using new method
Merge commit '9fc5c8aac7c0c57e78b1f75d0830ee8ea718f9cb' into aoai-acc…
9ed666f 
…ess-verification-rebranch
@christian-andersen-msft
Merge branch 'main' of https://github.com/microsoft/BCApps into aoai-…
da93eee 
…access-verification-rebranch
@christian-andersen-msft
Merge branch 'main' of https://github.com/microsoft/BCApps into aoai-…
0264589 
…access-verification-rebranch
@christian-andersen-msft
Simplified configuration check and made it more flexible
22af527
@christian-andersen-msft
Merge branch 'main' into aoai-access-verification-rebranch
f9abbb7
@christian-andersen-msft
Merge branch 'main' into aoai-access-verification-rebranch
5fc3ec2
@christian-andersen-msft
Temporary Database and notification templates added
2b18667
@christian-andersen-msft
tmp template changes
8a08eb2
@christian-andersen-msft
Grace period with caching added
d6c54fe
@christian-andersen-msft
Added notification and telemetry to verification logic as well as reo…
54ad50a 
…rganized function
@christian-andersen-msft
corercted comments
02bd29c
@christian-andersen-msft
added debugging messages
16e7169
@christian-andersen-msft
Merge branch 'main' into aoai-access-verification-rebranch
e3680d2
@christian-andersen-msft
removed wrong reference
8c41f6d
@christian-andersen-msft christian-andersen-msft requested a review from a team as a code owner January 14, 2025 12:31
@christian-andersen-msft christian-andersen-msft changed the title Aoai access verification rebranch Automatic access verification for AOAI services to develop and run on CAPI/managed AI resources Jan 14, 2025
@github-actions github-actions bot added this to the Version 26.0 milestone Jan 14, 2025
encimita
encimita reviewed Jan 15, 2025
View reviewed changes
src/System Application/App/AI/src/Azure OpenAI/AOAIAuthorization.Codeunit.al Show resolved Hide resolved
src/System Application/App/AI/src/Azure OpenAI/AOAIAuthorization.Codeunit.al Show resolved Hide resolved
src/System Application/App/AI/src/Azure OpenAI/AzureOpenAI.Codeunit.al Show resolved Hide resolved
src/System Application/App/AI/src/Azure OpenAI/AzureOpenAI.Codeunit.al Outdated Show resolved Hide resolved
src/System Application/App/AI/src/Azure OpenAI/AOAIAuthorization.Codeunit.al Outdated Show resolved Hide resolved
src/System Application/App/AI/src/Azure OpenAI/AOAIAuthorization.Codeunit.al Outdated Show resolved Hide resolved
src/System Application/App/AI/src/Azure OpenAI/AOAIAuthorization.Codeunit.al Outdated Show resolved Hide resolved
encimita
encimita requested changes Jan 28, 2025
View reviewed changes
src/System Application/App/AI/src/Azure OpenAI/AzureOpenAI.Codeunit.al Show resolved Hide resolved
src/System Application/App/AI/src/Azure OpenAI/AzureOpenAIImpl.Codeunit.al Show resolved Hide resolved
src/System Application/App/AI/src/Azure OpenAI/AOAIAuthorization.Codeunit.al
@@ -57,6 +77,19 @@ codeunit 7767 "AOAI Authorization"
Deployment := NewDeployment;
ApiKey := NewApiKey;
ManagedResourceDeployment := NewManagedResourceDeployment;
MicrosoftManagedAuthorizationWithDeployment := true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other overload of SetMicrosoftManagedAuthorization is now unused after the function in the other codeunit is removed.

So you need to wrap the old overload of SetMicrosoftManagedAuthorization into

#if not CLEAN26
    <old overload>
#endif

src/System Application/App/AI/src/Azure OpenAI/AOAIAuthorization.Codeunit.al
end
else
if MicrosoftManagedAuthorizationWithDeployment then
exit(AzureOpenAiImpl.IsTenantAllowlistedForFirstPartyCopilotCalls());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, we are no longer checking anywhere that the variables are not empty.

I suggest we don't add the 4 new booleans at all, and instead we rely on the existence of account name or not (for example).

Example pseudo-code:

Enum::"AOAI Resource Utilization"::"Microsoft Managed":
    if (AOAIAccountName <> '') and (ManagedResourceDeployment <> '')  and (not ApiKey.IsEmpty()) then
        exit(VerifyAOAIAccount(AOAIAccountName, ApiKey) and AzureOpenAiImpl.IsTenantAllowlistedForFirstPartyCopilotCalls())
    else
        exit((Deployment <> '') and (Endpoint <> '') and (not ApiKey.IsEmpty()) and (ManagedResourceDeployment <> '') and AzureOpenAiImpl.IsTenantAllowlistedForFirstPartyCopilotCalls());

You could even go one step further and make sure the old verification code is cleaned up automatically after the obsoletion period has passed

Example pseudo-code:

#if CLEAN26
    Enum::"AOAI Resource Utilization"::"Microsoft Managed":
        exit((AOAIAccountName <> '') and (ManagedResourceDeployment <> '')  and (not ApiKey.IsEmpty()) and VerifyAOAIAccount(AOAIAccountName, ApiKey) and AzureOpenAiImpl.IsTenantAllowlistedForFirstPartyCopilotCalls());
#else
    Enum::"AOAI Resource Utilization"::"Microsoft Managed":
        if (AOAIAccountName <> '') and (ManagedResourceDeployment <> '')  and (not ApiKey.IsEmpty()) then
            exit(VerifyAOAIAccount(AOAIAccountName, ApiKey) and AzureOpenAiImpl.IsTenantAllowlistedForFirstPartyCopilotCalls())
        else
            exit((Deployment <> '') and (Endpoint <> '') and (not ApiKey.IsEmpty()) and (ManagedResourceDeployment <> '') and AzureOpenAiImpl.IsTenantAllowlistedForFirstPartyCopilotCalls());
#endif

src/System Application/App/AI/src/Azure OpenAI/AOAIAuthorization.Codeunit.al
if VerificationLog.Get(TruncatedAccountName) then
RemainingGracePeriod := GracePeriod - (CurrentDateTime - VerificationLog.LastSuccessfulVerification)
else
RemainingGracePeriod := GracePeriod;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no entry in verification log, then the remaining grace period should be 0.

It means that azure account was never verified and hence they are not entitled to grace period.

src/System Application/App/AI/src/Azure OpenAI/AOAIAuthorization.Codeunit.al Show resolved Hide resolved
src/System Application/App/AI/src/Azure OpenAI/AOAIAuthorization.Codeunit.al Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darjoo darjoo darjoo left review comments

@msft-sam msft-sam msft-sam left review comments

@encimita encimita encimita requested changes

@NKarolak NKarolak NKarolak approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
AL: System Application
Projects
None yet
Milestone
Version 26.0
Development

Successfully merging this pull request may close these issues.
5 participants
@christian-andersen-msft @darjoo @NKarolak @encimita @msft-sam